Not sure I understand the purpose of this Makefile. It looks like it can be largely replaced with modern Python dev tools like uv and ruff (also curious why we're using black and isort here).

That's fair, I'll look into uv and ruff, and potentially delete this Makefile and update the README dev guide

I thought the idea was to publish this as a separate PyPI package? Why are we instructing users to build from source?

Ah, so this was moreso intended for the devs contributing to this package. I agree most customers would be installing it from PyPi. I can try to make that more clear in the next revision with adding clarifications to this README, let me know if you have additional asks for this

Why do we anticipate users to require dev dependencies?

Similar to my last comment, this workflow was intended for dev contributors (i.e. us). The dev dependencies are for PyTest and format packages to test and format the package.

Why do we have setup.py in addition to pyproject.toml?

Good point, after some research it seems we only really need one in our case, and that pyproject.toml is the favored way. I will delete setup.py in the next revision

Something that's confusing is that a line can show up as a diff even if nothing changed. Using the example file in this commit:

-aws secretsmanager put-secret-value --secret-id secret1213 \
-  --secret-binary file://data.json
+aws secretsmanager put-secret-value --secret-id secret1213 \
+  --secret-binary file://data.json --cli-binary-format raw-in-base64-out

Is this intended behavior?

Good point. I tried to keep the diff-generation code simple while also giving a somewhat familiar experience to git diff, and I think this behavior is an artifact of that.

We can pursue more complex diffing logic to address this. If we go down this route, I'd likely utilize difflib which is included in the standard Python library. It has a unified_diff function which may produce what we're looking for.

Below is some simple code I tested locally, seems to show the desired behavior:

>>> src_lines = 'aws secretsmanager put-secret-value --secret-id secret1213 \\\n  --secret-binary file://data.json\n'.splitlines(keepends=True)
>>> dest_lines = 'aws secretsmanager put-secret-value --secret-id secret1213 \\\n  --secret-binary file://data.json --cli-binary-format raw-in-base64-out\n'.splitlines(keepends=True)
>>> diff2 = difflib.unified_diff(src_lines, dest_lines)
>>> sys.stdout.writelines(diff2)
--- 
+++ 
@@ -1,2 +1,2 @@
 aws secretsmanager put-secret-value --secret-id secret1213 \
-  --secret-binary file://data.json
+  --secret-binary file://data.json --cli-binary-format raw-in-base64-out

I'm leaning towards going this route, please share feedback on this. It would likely simplify some of our diff-generation-code.

I think that looks great for our use case

If I'm understanding everything correctly, then src_lines_removed should always be equal to new_lines_added, right? If so, should we add validation on that to guard against potentially corrupting the source file?

As of now, yes I expect them to be equal. By calculating it like this it does not lock us in to that as a requirement.

I can imagine down the line us suggesting a 'fix' that takes up multiple lines, in that case they would not be equal. However, we currently have no plans for any of the linting rules to suggest fixes that take up multiple lines. It'll pretty much always be adding a flag to the last line or modifying the command or argument.

I can add this assertion, and there's always the option to remove it if we ever have suggestions for adding multi-line suggestions. Are you recommending it's added as a runtime check? Or introducing a test for this?

I'm leaning towards a runtime check, and raising RuntimeError if they're not equal, with comments explaining some things in this thread. That'll be my plan for next revision unless you share disagreements

Yeah I think we should just stick to line-by-line changes (ie always 1 line changed per 1 line affected). Go ahead and add the runtime check

It might be a bit confusing if a user accepts a few fixes and then presses q to exit early with the expectation that the accepted fixes would be applied.

• We should explicitly document this behavior.
• [non-blocking suggestion] And also consider adding a [s]ave option that "flushes" accepted fixes and applies them. If you think we should do this, okay with doing it in a followup PR.

This makes sense. Now that I think about this, I think we can let q be the save and exit option that you're suggesting. Users can always use Ctrl+C to terminate the program immediately.

Let me know if you agree with this. In either case, I would like to do it in a followup PR though.

Hmm let's discuss this with the team. I can see advantages to different approaches here

To calculate new_lines_added, is there a reason we're not using LintFinding.line_start and LintFinding.line_end?

I think this relates to a previous reply in another comment. I left open the possibility that there could be more "new lines" than "source lines". line_start and line_end are the line numbers in the source file.

If we end up deciding to be more rigid and force that the number of "new lines" equals the number of "source lines" with an assertion, we may be able use the variables you suggested instead.

With this information, please advise on if you want me to proceed with the more rigid approach, with the assertion that source == new lines.

Echoing my other reply: Yeah let's go ahead and make the assertion